Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: QUIC #282

Merged
merged 28 commits into from
Sep 2, 2024
Merged

feat: QUIC #282

merged 28 commits into from
Sep 2, 2024

Conversation

AsakuraMizu
Copy link
Collaborator

@AsakuraMizu AsakuraMizu commented Jul 31, 2024 •
edited
Loading

This PR introduces QUIC (and HTTP/3) support via quinn-proto.

Features ported from quinn-udp

  • GSO and GRO on linux and windows
  • Explicit Congestion Notification
  • Reporting the exact destination address of received packets and specifying explicit source addresses for sent packets
  • Disabled IP-layer fragmentation

Missing

  • Support for outdated operating systems or drivers
  • Batched recvmsg via recvmmsg (will explain later)

Features ported from quinn

Almost all features except:

  • NAT rebinding

Features ported from h3-quinn

  • HTTP/3 support via h3

Performance

quinn-udp uses recvmmsg syscall on supported platforms to improve performance. I did not port this feature as io_uring does not have corresponding opcode. As a result, our implementation is slightly slower than quinn when using io-uring driver (and of course much slower if using polling driver). Using multishot opcode may improve performance, but it's stilling pending (#104).
On platforms that does not support recvmmsg it still has some distance, and unfortunately benchmark program is not running on Windows. I think there may be some issues in my benchmark code.

Berrysoft
Berrysoft reviewed Jul 31, 2024
View reviewed changes
compio-quic/Cargo.toml Outdated Show resolved Hide resolved
@AsakuraMizu
Copy link
Collaborator Author

Some blocking issues:

  1. Dual-stack socket?
    Fact: IPV6_V6ONLY defaults to true only on Windows, and dual-stack is not supported by OpenBSD and FreeBSD. If we provide methods for creating dual-stack sockets, should we also provide methods for v6-only sockets?
    socket2 allows calling set_only_v6 after new and before bind, but our design does not welcome this.
    I prefer to make dual-stack the default on all platforms, or make it configurable via a feature flag.
  2. I/O functions on RecvStream/SendStream are not satisfactory. Specifically, SendStream::write will inevitably perform a copy, and the zero-copy method write_chunks requires the use of bytes. This is not friendly to our AsyncWrite trait, and I'm considering removing the impl. Similar case for RecvStream.

@AsakuraMizu AsakuraMizu marked this pull request as ready for review August 21, 2024 10:50
@AsakuraMizu AsakuraMizu changed the title QUIC feat: QUIC support Aug 21, 2024
@AsakuraMizu AsakuraMizu changed the title feat: QUIC support feat: QUIC Aug 21, 2024
Berrysoft
Berrysoft requested changes Aug 23, 2024
View reviewed changes
Copy link
Member

@Berrysoft Berrysoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job!

compio-driver/src/iocp/op.rs Outdated Show resolved Hide resolved
compio-driver/src/iocp/op.rs Outdated Show resolved Hide resolved
compio-net/tests/udp.rs Show resolved Hide resolved
compio/Cargo.toml Outdated Show resolved Hide resolved
compio-quic/Cargo.toml Outdated Show resolved Hide resolved
compio-quic/examples/client.rs Outdated Show resolved Hide resolved
Berrysoft
Berrysoft reviewed Aug 24, 2024
View reviewed changes
compio-driver/src/iocp/op.rs Outdated Show resolved Hide resolved
compio-driver/src/iocp/op.rs Outdated Show resolved Hide resolved
Berrysoft
Berrysoft reviewed Aug 24, 2024
View reviewed changes
compio-quic/Cargo.toml Outdated Show resolved Hide resolved
@Berrysoft
Copy link
Member

Check the benchmark, please. It stack overflows on my Windows machine.

@AsakuraMizu
Copy link
Collaborator Author

AsakuraMizu commented Aug 24, 2024 •
edited
Loading

Check the benchmark, please. It stack overflows on my Windows machine.

It turns out that the data is just too big.
But while adjusting the size I found that different sizes have a big impact on the performance... (I mean, the gap between our implementation and quinn)
For example if large_data has a length of 1024 * 64=65536 compio-quic is 1.4x faster than quinn on echo-large-data-1-stream. But if large_data has a length of 1024 * 16=16384 then quinn is 0.7x faster than us on the same testcase. (These data come from runs on Windows)
Maybe the benchmarks still have a lot to improve.

Berrysoft
Berrysoft approved these changes Aug 24, 2024
View reviewed changes
Copy link
Member

@Berrysoft Berrysoft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

George-Miao
George-Miao reviewed Aug 26, 2024
View reviewed changes
compio-net/src/udp.rs Outdated Show resolved Hide resolved
@George-Miao George-Miao added enhancement New feature or request package: quic Related to compio-quic labels Aug 26, 2024
George-Miao
George-Miao approved these changes Sep 2, 2024
View reviewed changes
Copy link
Member

@George-Miao George-Miao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@George-Miao George-Miao merged commit 111c8d0 into compio-rs:master Sep 2, 2024
29 checks passed
@Berrysoft Berrysoft mentioned this pull request Sep 9, 2024
Closed
@AsakuraMizu AsakuraMizu deleted the quic branch October 20, 2024 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@George-Miao George-Miao George-Miao approved these changes

@Berrysoft Berrysoft Berrysoft approved these changes

Assignees

@AsakuraMizu AsakuraMizu

Labels
enhancement New feature or request package: quic Related to compio-quic
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AsakuraMizu @Berrysoft @George-Miao